Skip to content

feat: add QuadraticDiophantineEquations model (#543)#812

Merged
isPANN merged 8 commits intomainfrom
543-quadratic-diophantine-equations
Mar 29, 2026
Merged

feat: add QuadraticDiophantineEquations model (#543)#812
isPANN merged 8 commits intomainfrom
543-quadratic-diophantine-equations

Conversation

@isPANN
Copy link
Copy Markdown
Collaborator

@isPANN isPANN commented Mar 28, 2026

Summary

  • Add QuadraticDiophantineEquations model: given positive integers a, b, c, decide whether ax^2 + by = c has a solution in positive integers x, y
  • Single variable x (y is derived); domain {1,...,floor(sqrt(c/a))}; Value type Or
  • Full CLI support (pred create QuadraticDiophantineEquations --coeff-a --coeff-b --rhs), canonical example, paper entry with worked table
  • 15 unit tests covering creation, evaluation, solver, serialization, edge cases

Closes #543

Test plan

  • All 15 model unit tests pass (basic, evaluate yes/no, solver witness/all/none, serialization, deserialization rejection, check_x, edge cases, paper example, panic tests)
  • make fmt-check clean
  • make clippy clean
  • make test full suite passes

🤖 Generated with Claude Code

Add decision problem: given positive integers a, b, c, determine whether
there exist positive integers x, y such that ax^2 + by = c. Single
variable x with y derived; complexity O(sqrt(c)) by trial.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 28, 2026

Codecov Report

❌ Patch coverage is 97.08738% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.86%. Comparing base (102b941) to head (79eae42).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...odels/algebraic/quadratic_diophantine_equations.rs 93.87% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #812      +/-   ##
==========================================
- Coverage   97.86%   97.86%   -0.01%     
==========================================
  Files         635      637       +2     
  Lines       69612    69818     +206     
==========================================
+ Hits        68126    68325     +199     
- Misses       1486     1493       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@GiggleLiu
Copy link
Copy Markdown
Contributor

Agentic Review Report

Structural Check

Structural Review: model QuadraticDiophantineEquations

Structural Completeness

# Check Status
1 Model file exists PASS — src/models/algebraic/quadratic_diophantine_equations.rs is present
2 inventory::submit! present PASS — schema and size-field registrations are present at src/models/algebraic/quadratic_diophantine_equations.rs:12 and :28
3 Serialize / Deserialize support PASS — Serialize is derived on the struct and custom Deserialize funnels through try_new() at src/models/algebraic/quadratic_diophantine_equations.rs:162
4 Problem trait impl PASS — impl Problem for QuadraticDiophantineEquations at src/models/algebraic/quadratic_diophantine_equations.rs:172
5 Aggregate value is present PASS — type Value = Or at src/models/algebraic/quadratic_diophantine_equations.rs:174
6 #[cfg(test)] + #[path = ...] test link PASS — linked test module at src/models/algebraic/quadratic_diophantine_equations.rs:217
7 Test file exists PASS — src/unit_tests/models/algebraic/quadratic_diophantine_equations.rs exists
8 Test file has >= 3 test functions PASS — 14 test functions in src/unit_tests/models/algebraic/quadratic_diophantine_equations.rs
9 Registered in algebraic/mod.rs PASS — module and example-spec aggregation added in src/models/algebraic/mod.rs
10 Re-exported in models/mod.rs PASS — re-export added in src/models/mod.rs:14
11 Variant registration exists PASS — crate::declare_variants! at src/models/algebraic/quadratic_diophantine_equations.rs:202 (the packet's missing declare_variants appears to be a false positive)
12 CLI resolve_alias entry PASS — alias QDE is registered in schema and resolved through the registry-backed problem_name.rs path
13 CLI create support PASS — problemreductions-cli/src/commands/create.rs:2374 wires explicit construction and problemreductions-cli/src/cli.rs:736 adds the flags
14 Canonical model example registered PASS — src/models/algebraic/mod.rs:47 includes the new model's canonical example specs, and src/example_db/model_builders.rs already chains all algebraic specs
15 Paper display-name entry PASS — docs/paper/reductions.typ:177
16 Paper problem-def block PASS — docs/paper/reductions.typ:3274

Blacklisted Files

  • PASS — no blacklisted autogenerated files were committed

Build Status

  • make test: PASS
  • make clippy: PASS

Semantic Review

  • evaluate() correctness: OK — the model enumerates only x, derives y from c - a*x^2, rejects non-positive and non-divisible remainders, and returns Or(false) for invalid configs.
  • dims() correctness: OK — vec![max_x] matches the x-only search space, and vec![0] correctly yields zero configurations under DimsIterator when c < a.
  • Size getter consistency: ISSUE — the user-facing size metadata exposes only c_val, which leaks an implementation detail and diverges from the linked-issue bundle's mathematical a, b, c naming.
  • Weight handling: OK — not applicable for this unweighted feasibility model.

Issue Compliance

# Check Status
1 Problem name matches issue OK
2 Mathematical definition matches OK
3 Problem framing matches OK — feasibility / witness model with Value = Or
4 Type parameters match OK — no variants/type parameters, matching the issue
5 Configuration space matches OK — x-only encoding matches the issue's stated strategy that y is derived
6 Feasibility check matches OK
7 Objective function matches OK — returns true iff a positive-integer solution exists
8 Complexity matches OK — brute-force bound matches the issue, though the exposed name is sqrt(c_val) rather than sqrt(c)

Summary

  • 16/16 structural checks passed
  • 8/8 issue-compliance checks passed
  • ISSUE — size metadata exposes c_val instead of the mathematical parameter naming used in the issue packet

Quality Check

Quality Review

Design Principles

  • DRY: OK — the implementation is self-contained and follows existing model/create wiring patterns without unnecessary duplication.
  • KISS: OK — encoding only x and deriving y is the simplest faithful representation of the problem.
  • HC/LC: OK — model logic, CLI wiring, docs, and tests remain in their expected modules.

HCI (if CLI/MCP changed)

  • Error messages: OK — pred create QuadraticDiophantineEquations --coeff-a 3 fails with an actionable missing-flag message, and failed default solve points users to --solver brute-force.
  • Discoverability: ISSUE — pred list and pred show QuadraticDiophantineEquations expose O(sqrt(c_val)) and Size fields: c_val, leaking an implementation-specific getter name into user-facing catalog output (src/models/algebraic/quadratic_diophantine_equations.rs:29, :102, :202).
  • Consistency: ISSUE — the new paper example advertises pred solve qde.json, but the CLI default solver is ILP and this model has no ILP reduction path (docs/paper/reductions.typ:3281).
  • Least surprise: ISSUE — following the documented command fails with No reduction path from QuadraticDiophantineEquations to ILP; the working command is pred solve qde.json --solver brute-force.
  • Feedback: OK — the solve failure includes the correct brute-force hint.

Test Quality

  • Naive test detection: ISSUE
    • The added unit tests cover model semantics well, but none of the added coverage exercises the changed CLI/help/doc path, so the broken pred solve qde.json example and the c_val metadata leak were not caught before review.

Issues

Critical (Must Fix)

  • docs/paper/reductions.typ:3281 documents pred solve qde.json, but that exact command fails on this PR because QuadraticDiophantineEquations has no reduction path to ILP. Reproduced with target/debug/pred solve /tmp/qde-review-812.json -> Error: No reduction path from QuadraticDiophantineEquations to ILP.

Important (Should Fix)

  • src/models/algebraic/quadratic_diophantine_equations.rs:29, :102, and :202 make user-facing catalog output show c_val instead of the mathematical parameter name c. Reproduced in both pred list and pred show QuadraticDiophantineEquations.
  • The changed CLI/doc flow lacks automated coverage: no test currently exercises the advertised solve command for this new model.

Minor (Nice to Have)

  • None.

Summary

  • Documented default solve path is broken for the new model because no ILP route exists.
  • User-facing catalog metadata leaks the internal getter name c_val.
  • Added tests are strong for model semantics, but they miss the changed CLI/doc workflow.

Agentic Feature Tests

Feature Test Report: problem-reductions

Date: 2026-03-29
Project type: Rust library + CLI
Features tested: QuadraticDiophantineEquations model
Profile: ephemeral
Use Case: A downstream CLI user wants to discover the new model, inspect it, create the documented example instance, and solve it using the commands shown in project docs.
Expected Outcome: The model should appear in the catalog, pred show should describe it cleanly, pred create --example QuadraticDiophantineEquations should produce a valid instance, and the paper's pred solve qde.json command should solve that instance.
Verdict: fail
Critical Issues: 1

Summary

Feature Discoverable Setup Works Expected Outcome Met Doc Quality
QuadraticDiophantineEquations model partial yes partial no broken solve command; c_val leaks into UI

Per-Feature Details

QuadraticDiophantineEquations model

  • Role: downstream CLI user testing a newly added model from docs alone
  • Use Case: inspect the model, create the documented example, and solve it
  • What they tried: target/debug/pred list; target/debug/pred show QuadraticDiophantineEquations; target/debug/pred show QDE; target/debug/pred create --example QuadraticDiophantineEquations -o /tmp/qde-review-812.json; target/debug/pred create QuadraticDiophantineEquations --coeff-a 3 --coeff-b 5 --rhs 53 -o /tmp/qde-manual-812.json; target/debug/pred solve /tmp/qde-review-812.json; target/debug/pred solve /tmp/qde-review-812.json --solver brute-force; target/debug/pred evaluate /tmp/qde-review-812.json --config 0
  • Discoverability: Partial. The model is present in pred list, and alias QDE resolves correctly, but pred list / pred show surface c_val, which looks like an internal API symbol rather than the mathematical parameter c.
  • Setup: Yes. No extra setup was needed once the workspace binary was built.
  • Functionality: Partial. Example creation, manual creation, alias resolution, evaluation, and brute-force solve all work. Default solve does not.
  • Expected vs Actual Outcome: Expected the paper's pred solve qde.json example to work verbatim. Actual result: that command fails because the default ILP solver has no path from QuadraticDiophantineEquations to ILP; adding --solver brute-force makes the example work.
  • Blocked steps: None.
  • Friction points: The documented solve command is wrong for this model; user-facing output leaks c_val.
  • Doc suggestions: Update the paper command to pred solve qde.json --solver brute-force (or add a solve path/default fallback), and expose c rather than c_val in catalog metadata.

Expected vs Actual Outcome

  • Example creation: achieved
  • Manual create with explicit flags: achieved
  • Example solve as documented: not achieved
  • Example solve with explicit brute-force solver: achieved

Issues Found

  1. Confirmed — Critical: pred solve qde.json fails because there is no ILP reduction path for this model.
  2. Confirmed — Important: pred list / pred show leak the internal getter name c_val into user-facing output.

Suggestions

  • Fix the paper example or solver behavior so the documented solve path works.
  • Clean up size-field / complexity naming so user-facing output uses mathematical terms.

Generated by review-pipeline

@isPANN isPANN mentioned this pull request Mar 29, 2026
3 tasks
…mand

- Rename `c_val()` getter to `c()` so user-facing catalog output shows
  the mathematical parameter name instead of an implementation detail
- Add `--solver brute-force` to paper's `pred solve` command since
  QuadraticDiophantineEquations has no ILP reduction path

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@zazabap zazabap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

zazabap and others added 4 commits March 29, 2026 16:45
The file was brought in by a contaminated merge commit and does not
belong to the QuadraticDiophantineEquations model PR.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
All three QDE parameters now use the coeff- prefix: --coeff-a, --coeff-b, --coeff-c.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@isPANN isPANN merged commit 7657a08 into main Mar 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Model] QuadraticDiophantineEquations

3 participants